-
Notifications
You must be signed in to change notification settings - Fork 237
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start 👍
pkg/options/operator/options.go
Outdated
MySQLAgentImage string `yaml:"mysqlAgent"` | ||
MySQLServerImage string `yaml:"mysqlServer"` | ||
MySQLAgentImage string `yaml:"mysqlAgent"` | ||
ImagePullSecret *v1.LocalObjectReference `yaml:"imagePullSecret"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be configured on the MySQLCluster CRD IMO 🙂
data: | ||
mysql-operator-config.yaml: | | ||
images: | ||
mysqlServer: store/oracle/mysql-enterprise-server #Enterprise sql image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add an Image
field to the Cluster CRD to let users choose enterprise on a per MySQL cluster basis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea here as it brings the benefit of having a per cluster MySQL and also having patched custom MySQL images. It's almost a separate PR and feature in it's own right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When going down this route we'll need to double check and make sure we update documentation and check existing code doesn't reference two sources of the image.
@@ -0,0 +1,287 @@ | |||
# 01 - Custom resources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to need to maintain another set of manifests. We should add an option to the helm chart for setting the default image that the operator uses and then get users to install via helm.
7c4372a
to
d5d2694
Compare
|
||
func TestClusterWithOnlyMysqlServerResourceRequirements(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to delete these tests?
@@ -223,67 +224,34 @@ func TestClusterWithTolerations(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestClusterWithResourceRequirements(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to delete these tests?
docs/enterprise-edition-example.md
Outdated
@@ -0,0 +1,54 @@ | |||
# Enterprise edition tutorial | |||
This tutorial will explain how to create a mysqlcluster that runs the enterprise version of mysql. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mysqlcluster => MySQL cluster (?)
mysql => MySQL
docs/enterprise-edition-example.md
Outdated
## Prerequisites | ||
|
||
- The mysql-operator repository checked out locally. | ||
- Access to a Docker registry that contains the enterprise version of mysql. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mysql => MySQL
docs/enterprise-edition-example.md
Outdated
The creation of these resources can be achieved by following the [introductory tutorial][1]; return here before creating a MySQL Cluster. | ||
|
||
## 02 - Create a secret with registry credentials | ||
To be able to pull the mysql enterprise edition from docker it is necessary to provide credentials, these credentials must be supplied in the form of a Kubernetes secret. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mysql enterprise edition => MySQL Enterprise Edition
docker => Docker
docs/enterprise-edition-example.md
Outdated
|
||
- The name of the secret `myregistrykey` must match the name in the `imagepullsecrets` which we will specify in the cluster config in step 03. | ||
- The secret must be created in the same namespace as the MySQL Cluster which we will make in step 03. It must also be in the same namespace as the RBAC permissions created in step 01. | ||
- If you are pulling the mysql enterprise image from a different registry then the secret must contain the relevant credentials for that registry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mysql enterprise => MySQL Enterprise
docs/enterprise-edition-example.md
Outdated
- The secret must be created in the same namespace as the MySQL Cluster which we will make in step 03. It must also be in the same namespace as the RBAC permissions created in step 01. | ||
- If you are pulling the mysql enterprise image from a different registry then the secret must contain the relevant credentials for that registry. | ||
|
||
>For alternative ways to create Kubernetes secretes see their documentation on [creating secrets from docker configs](https://kubernetes.io/docs/concepts/containers/images/#specifying-imagepullsecrets-on-a-pod) or [creating secrets manually](https://kubernetes.io/docs/concepts/containers/images/#creating-a-secret-with-a-docker-config). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: secretes => secrets
docs/enterprise-edition-example.md
Outdated
|
||
>For alternative ways to create Kubernetes secretes see their documentation on [creating secrets from docker configs](https://kubernetes.io/docs/concepts/containers/images/#specifying-imagepullsecrets-on-a-pod) or [creating secrets manually](https://kubernetes.io/docs/concepts/containers/images/#creating-a-secret-with-a-docker-config). | ||
|
||
Enter your credentials into the following command and execute it to create a Kubernetes secret that will enable pulling images from the Docker store. add the `-n` flag to specify a namespace if you do not want to use the default namespace. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. add the => Add the ...
docs/enterprise-edition-example.md
Outdated
Finally, create your MySQL Cluster with the required specifications entered under `spec:` | ||
|
||
- The mysqlServer field should be the path to a registry containing the enterprise edition of MySQL. | ||
- The imagePullSecret: name: Should be the name of a Kubernetes secret in the same namespace that contains your credentials for the docker registry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker registry => Docker registry
docs/enterprise-edition-example.md
Outdated
kubectl apply -f examples/cluster/cluster-enterprise-version.yaml | ||
``` | ||
### Check that it is running | ||
You can now run the following command to access the sql prompt in your MySQL Cluster, just replace `<NAMESPACE>` with the namespace you created your cluster in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sql => SQL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the design around setting the pull secret and image as part of the cluster spec. Some minor comments but overall looking great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good 👍
pkg/apis/mysql/v1alpha1/types.go
Outdated
@@ -27,6 +28,11 @@ const MinimumMySQLVersion = "8.0.11" | |||
type ClusterSpec struct { | |||
// Version defines the MySQL Docker image version. | |||
Version string `json:"version"` | |||
// MySQLServerImage defines the image to be pulled for the mysqlServer. | |||
MySQLServerImage string `json:"mysqlServer"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we call this field Repository
pkg/apis/mysql/v1alpha1/types.go
Outdated
MySQLServerImage string `json:"mysqlServer"` | ||
// ImagePullSecret defines the name of the secret that contains the | ||
// required credentials for pulling the MySQLServerImage. | ||
ImagePullSecret *v1.LocalObjectReference `json:"imagePullSecret"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we implement as []v1.LocalObjectReference
as in the core.
From v1.PodSpec
:
// ImagePullSecrets is an optional list of references to secrets in the same namespace to use for pulling any of the images used by this PodSpec.
// If specified, these secrets will be passed to individual puller implementations for them to use. For example,
// in the case of docker, only DockerConfig type secrets are honored.
// More info: https://kubernetes.io/docs/concepts/containers/images#specifying-imagepullsecrets-on-a-pod
// +optional
// +patchMergeKey=name
// +patchStrategy=merge
ImagePullSecrets []LocalObjectReference `json:"imagePullSecrets,omitempty" patchStrategy:"merge" patchMergeKey:"name" protobuf:"bytes,15,rep,name=imagePullSecrets"`
6576170
to
eb5daf6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nearly there 😄
pkg/apis/mysql/v1alpha1/types.go
Outdated
@@ -15,6 +15,7 @@ | |||
package v1alpha1 | |||
|
|||
import ( | |||
"k8s.io/api/core/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already imported as corev1
below
pkg/apis/mysql/v1alpha1/types.go
Outdated
@@ -27,6 +28,11 @@ const MinimumMySQLVersion = "8.0.11" | |||
type ClusterSpec struct { | |||
// Version defines the MySQL Docker image version. | |||
Version string `json:"version"` | |||
// Repository defines the image to be pulled for the MySQL server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/image/image repository/
pkg/options/operator/options.go
Outdated
mysqlAgent = "iad.ocir.io/oracle/mysql-agent" | ||
mysqlServer = "mysql/mysql-server" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of redefining this here use the value from the v1alpha1
pkg.
@@ -344,8 +344,15 @@ func NewForCluster(cluster *v1alpha1.Cluster, images operatoropts.Images, servic | |||
}) | |||
} | |||
|
|||
var serverImage string | |||
if cluster.Spec.Repository != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This defaulting will be redundant if we can manage to default in Cluster.EnsureDefaults()
pkg/options/operator/options.go
Outdated
@@ -122,7 +123,7 @@ func (s *MySQLOperatorOpts) AddFlags(fs *pflag.FlagSet) *pflag.FlagSet { | |||
fs.StringVar(&s.KubeConfig, "kubeconfig", s.KubeConfig, "Path to Kubeconfig file with authorization and master location information.") | |||
fs.StringVar(&s.Master, "master", s.Master, "The address of the Kubernetes API server (overrides any value in kubeconfig).") | |||
fs.StringVar(&s.Namespace, "namespace", metav1.NamespaceAll, "The namespace for which the MySQL operator manages MySQL clusters. Defaults to all.") | |||
fs.StringVar(&s.Images.MySQLServerImage, "mysql-server-image", s.Images.MySQLServerImage, "The name of the target 'mysql-server' image. Defaults to: mysql/mysql-server.") | |||
fs.StringVar(&s.Images.DefaultMySQLServerImage, "mysql-server-image", mysqlServer, "The name of the default target for the 'mysql-server' image (can be overridden on a per-cluster basis). Defaults to: "+mysqlServer+".") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/mysqlServer/s.Images.DefaultMySQLServerImage/
…operator into bcb/SupportEnterpriseMYSQL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny nit pick but once that's cleaned up go ahead and merge - awesome work 🎉
Another thought: Should we include an E2E test or two for enterprise?
defaultMembers = 3 | ||
defaultBaseServerID = 1000 | ||
// maxBaseServerID is the maximum safe value for BaseServerID calculated | ||
// as max MySQL server_id value - max Replication Group size. | ||
maxBaseServerID uint32 = 4294967295 - 9 | ||
// MysqlServer is the image to use if no image is specified explicitly by the user. | ||
MysqlServer = "mysql/mysql-server" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/MysqlServer/DefaultRepository/
These changes introduce the ability for users to use the enterprise edition of the MySQL server.
This allows for pulling of any valid MySQL image as long as the correct credentials are provided as a Kubernetes secret.
Images are now specified on a per-cluster basis allowing for one operator to handle creation of various MySQL Clusters.